chore: fold in 5 safe review-findings fixes (dedup/stale-doc/dead-code) - #418
Merged
Merged
Conversation
From the cross-repo dead-code/duplication review:
- sketch_reducer.rs: the FrequencyTopk `topk` sort (summed counts from a
per-window HashMap) had no tie-break, so equal-count keys ordered
non-deterministically across runs. Add the same deterministic
key-ascending tie-break summary_executor.rs's topk_ranked already has.
- cost_model.rs: fix a stale doc claiming a `BindCountSketchOnTopK`
pre-pass still binds `TopK{accuracy: Exact}` in lower.rs -- that rule
was deleted; lower.rs's own doc already says the correct thing
(falls through to implement_tree_in_with's Logical fallback, a
genuine open asap-plan gap, ASAPController#151).
- optimizer/rules/mod.rs: delete dead `ENV_USE_TYPED_SKETCH_ALGEBRA`/
`typed_sketch_algebra_enabled()` (zero callers), and fix
sketch_algebra/mod.rs's crate doc, which still claimed this env var
gates the typed L3->L4 binding path -- that binding actually runs
unconditionally from main.rs today; the real env gate in this area
(`USE_TYPED_STAGE_SPLIT`) is on the downstream L4->L5 stage-split step.
- sketch_reducer.rs: `evaluate`'s sister doc said the string-dispatch
entry point is "retained for legacy callers" -- there are none in
production (engine.rs calls the typed `evaluate_for_capability`
exclusively); it's kept because the query-path test suite still
exercises it via PromQL function-name strings. Clarified rather than
migrating 18 test call sites (out of scope for this round).
- http.rs: delete ~120 lines of commented-out dead
PrometheusResponse/format_results_as_http_response code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 27, 2026
…t plan Rebased onto current main (was 27 commits behind, predating #416/#417/#418) and updated the doc's content to match: - Status note: SummaryExecutor now covers the Frequency family and ExactAgg(Sum/Increase) candidate matching + coverage tracking, not just quantile/cardinality -- these landed since the doc was last synced. - "Today"/gap list: gap 2 (merge only existed for ExactAgg) and gap 3 (nothing checked param agreement) are now resolved on the new path, marked accordingly rather than left as open problems. - SidHandle/GroupState table entries: updated to describe the actual enum shape (Sketch/ExactAgg variants), not the original sketch-only struct design. - New "Architecture reference" section citing ASAPController design.md's "Serving-time execution" section directly -- the planning-vs-serving split this doc's Rollout section builds on. - "Rollout" section: replaces "still open" with an actual plan. Corrects a real error in the previous version -- it named `implement_promql_for_asap_tier` as "the seam" for tree construction, but that function uses the naive DefaultCostModel and has a documented, tracked gap where it can't realize the Frequency intent at all. The correct seam is `sketch_algebra::lower::bind_query_expr` (ControlPlaneCostModel, what main.rs's real production pipeline uses). Also documents why rate()/topk-over-rate/outer-agg-fold must be excluded from the shadow comparison entirely (not just deprioritized) and why shadow mode -- not a cutover -- is this phase's actual scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Small, independent fixes from the cross-repo dead-code/duplication review — findings #2, #3, #4, #5, #9 (the ones judged safe/mechanical; #6-#8 were info-only, #1 landed separately in #416):
sketch_reducer.rs'sFrequencyTopksort had no deterministic tie-break for equal counts (fed by aHashMap's non-deterministic iteration order). Added the same key-ascending tie-breaksummary_executor.rs'stopk_rankedalready has.cost_model.rshad a stale doc claiming aBindCountSketchOnTopKpre-pass still bindsTopK{accuracy: Exact}— that rule was deleted; fixed to matchlower.rs's own (correct) doc.ENV_USE_TYPED_SKETCH_ALGEBRA/typed_sketch_algebra_enabled()(zero callers), and fixedsketch_algebra/mod.rs's crate doc, which claimed this env var gates the typed L3→L4 binding path — that binding actually runs unconditionally frommain.rs; the real gate in this area (USE_TYPED_STAGE_SPLIT) is on the downstream L4→L5 stage-split step.evaluate_for_capability's doc — the string-dispatchevaluatesister has zero production callers (engine.rscalls the typed entry exclusively) but is kept because the query-path test suite (18 sites) still exercises it via PromQL function-name strings. Migrating those 18 sites is out of scope for this round.PrometheusResponse/format_results_as_http_responsecode inhttp.rs.Test plan
cargo build --workspace— cleancargo test -p data_plane --lib— 913 passedcargo test -p control_plane --lib— 766 passed, 1 known pre-existing failure (invalid_sketch_type_override_falls_back_to_default), confirmed identical on origin/maincargo clippy -p data_plane --lib/-p control_plane --lib— identical warning counts to baselinerustfmton touched files only🤖 Generated with Claude Code